Fix for having two or more links in normative text#137
Fix for having two or more links in normative text#137james-ball-qualcomm merged 1 commit intomainfrom
Conversation
Signed-off-by: James Ball <jameball@qti.qualcomm.com>
james-ball-qualcomm
deleted the
136-adoc-link-replacement-to-html-bug
branch
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the normative rules generation where having two or more links in the same normative text would be incorrectly processed. The previous implementation used greedy regex patterns that would match from the first << to the last >>, treating everything in between as a single link. The fix changes to a non-greedy pattern and consolidates the link parsing logic.
Key Changes:
- Replaced two separate regex patterns (for links with and without custom text) with a single non-greedy pattern
- Implemented comma-splitting logic to distinguish between simple links and links with custom text
- Added test cases for multiple links in the same normative text (both with and without custom text)
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/create_normative_rules.rb | Refactored link parsing to use non-greedy regex and consolidated the two separate gsub calls into one with comma-based parsing |
| tests/norm-rule/test.yaml | Added hyperlink5 and hyperlink6 test definitions |
| tests/norm-rule/test.adoc | Added test cases with multiple links in the same normative text |
| tests/norm-rule/expected/test-norm-tags.json | Updated expected JSON output with new test cases |
| tests/norm-rule/expected/test-norm-rules.xlsx | Updated expected Excel output (binary file) |
| tests/norm-rule/expected/test-norm-rules.json | Updated expected JSON rules output with new test cases |
| tests/norm-rule/expected/test-norm-rules.html | Updated expected HTML output showing properly parsed multiple links |
| tests/norm-rule/expected/test-norm-rules.adoc | Updated expected AsciiDoc output with new test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elsif split_texts.length == 2 | ||
| tag2html_link(split_texts[0], split_texts[1], html_fname) | ||
| else | ||
| fail("Hyperlink '$1' contains too many commas") |
There was a problem hiding this comment.
The error message uses single quotes, which prevents Ruby from interpolating $1. The message will literally display "$1" instead of the actual hyperlink content. Change to double quotes: "Hyperlink \"#{$1}\" contains too many commas" or use "Hyperlink '#{$1}' contains too many commas" for proper interpolation.
| fail("Hyperlink '$1' contains too many commas") | |
| fail("Hyperlink '#{$1}' contains too many commas") |
| # will return multiple <<link>> in the same text as one. | ||
| tag_text.gsub!(/#{LT_UNICODE_STR}#{LT_UNICODE_STR}(.+?)#{GT_UNICODE_STR}#{GT_UNICODE_STR}/) do | ||
| # Look to see if custom text has been provided. | ||
| split_texts = $1.split(",") |
There was a problem hiding this comment.
Consider trimming whitespace from split_texts elements before passing them to tag2html_link. Currently, <<link , custom text>> (with space before comma) would result in "link " being used as the tag reference, which could cause issues. Adding .map(&:strip) after the split would handle this: split_texts = $1.split(",").map(&:strip)
| split_texts = $1.split(",") | |
| split_texts = $1.split(",").map(&:strip) |
| if split_texts.length == 0 | ||
| fail("Hyperlink '$1' is empty") | ||
| elsif split_texts.length == 1 |
There was a problem hiding this comment.
The condition split_texts.length == 0 is impossible to reach. When splitting a non-empty string (captured by .+?), the result will always have at least one element. Consider removing this dead code or replacing it with a more meaningful check.
| if split_texts.length == 0 | |
| fail("Hyperlink '$1' is empty") | |
| elsif split_texts.length == 1 | |
| if split_texts.length == 1 |
| split_texts = $1.split(",") | ||
|
|
||
| if split_texts.length == 0 | ||
| fail("Hyperlink '$1' is empty") |
There was a problem hiding this comment.
The error message uses single quotes, which prevents Ruby from interpolating $1. The message will literally display "$1" instead of the actual hyperlink content. Change to double quotes: "Hyperlink \"#{$1}\" is empty" or use "Hyperlink '#{$1}' is empty" for proper interpolation.
| fail("Hyperlink '$1' is empty") | |
| fail("Hyperlink '#{$1}' is empty") |
No description provided.